New lint [local_assigned_single_value]
#10977
Conversation
|
r? @giraffate (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
r? @Jarcho (apparently you know MIR?) |
Centri3
force-pushed
the
local_assignment_single_value
branch
from
9a054f6
to
9877a96
Compare
Centri3
force-pushed
the
local_assignment_single_value
branch
from
9877a96
to
3413ae9
Compare
|
The implementation is pretty much done now, I may refactor again though. I still see no FPs as it's overly pessimistic, if it can't prove with 100% confidence it's constant it won't lint. There are still missed cases, though, like casting a C-like enum or getting the field of a constant ADT |
Centri3
force-pushed
the
local_assignment_single_value
branch
from
96cbee6
to
4853c84
Compare
|
☔ The latest upstream changes (presumably #10951) made this pull request unmergeable. Please resolve the merge conflicts. |
Centri3
force-pushed
the
local_assignment_single_value
branch
from
4853c84
to
f15e601
Compare
|
☔ The latest upstream changes (presumably #10884) made this pull request unmergeable. Please resolve the merge conflicts. |
Centri3
force-pushed
the
local_assignment_single_value
branch
from
f15e601
to
d406752
Compare
Centri3
force-pushed
the
local_assignment_single_value
branch
from
d406752
to
5643945
Compare
|
|
||
| /// Holds the data we have for the usage of a local. | ||
| #[derive(Debug, Default)] | ||
| struct LocalUsageValues<'tcx> { |
There was a problem hiding this comment.
This is better represented as an enum. Something like:
enum State {
Uninitialized,
SingleValue(Value, Vec<Span>),
MultiValue,
}| let mut v = V { | ||
| body: mir, | ||
| cx, | ||
| map: mir.local_decls.iter().map(|_| LocalUsageValues::default()).collect(), |
There was a problem hiding this comment.
The map should be initialized to a state such that only user bound mutable variables check the assigned value. i.e. matches!(l.local_info, LocalInfo::User(_)) && l.mutability == Mutability::Mut)
There was a problem hiding this comment.
Well, local_info is actually ClearCrossCrate now so we can't. It's not available past borrowck and friends and results in an ICE if used in clippy.
We could perhaps use var_debug_info or something, or LocalKind::Temp.
| return; | ||
| }; | ||
|
|
||
| if stmt.source_info.span.from_expansion() { |
There was a problem hiding this comment.
You can't just ignore assignments from macros. The most permissive thing you can do here is to block linting for any locals which are assigned in macros. Trying to lint macros will be pain if you want to avoid false positives.
| span: stmt.source_info.span, | ||
| }; | ||
|
|
||
| if let Rvalue::Use(operand) = rvalue { |
There was a problem hiding this comment.
This whole chain should just be a match.
| // Handle `let [x, y] = [1, 1]` and `let (x, y) = (1, 1)` | ||
| && matches!(base_proj, PlaceElem::Field(..) | PlaceElem::Index(..)) | ||
| { | ||
| spanned.node = LocalUsage::DependsOnWithProj(place.local, base_proj); |
There was a problem hiding this comment.
This is only ok if the source projection is not mutable.
| } else { | ||
| // It seems sometimes a local's just moved, no projections, so let's make sure we | ||
| // don't set `assigned_non_const` to true for these cases | ||
| spanned.node = LocalUsage::DependsOn(place.local); |
There was a problem hiding this comment.
This is only ok if the source projection is not mutable.
| && matches!(base_proj, PlaceElem::Field(..) | PlaceElem::Index(..)) | ||
| { | ||
| // Probably unnecessary | ||
| spanned.node = LocalUsage::DependsOnWithProj(place.local, base_proj); |
There was a problem hiding this comment.
This is only ok if the source projection is not mutable.
| spanned.node = LocalUsage::DependsOnWithProj(place.local, base_proj); | ||
| } else { | ||
| // Handle casts from enum discriminants | ||
| spanned.node = LocalUsage::DependsOn(place.local); |
There was a problem hiding this comment.
This is only ok if the source projection is not mutable.
|
Thanks for reviewing! I agree that there's a ton of work to do on this. I'll probably rewrite what you didn't review yet as it's even uglier. Unfortunately I might not be able to get to this as I've been having issues with both my physical and mental health, but I'll get to it in due time ^^ |
|
I'm closing this for now as I have no intention to come back to it. If anybody else wants to work on this in the next few months, you're free to. Some things I feel like should be note to anybody working on this in the future: MIR might not be the best for this. Currently, let mut x = 1;
let mut y = 1;
x = y;
y = x;Will either get into a loop or not lint, despite we should lint it. If this operates on the HIR, we can ignore let (&&&x, [y, z], A { x: _, w, .. }) = (&&&&&&1, [1, 2], A { x: 0, y: 0, z: 0, w: 0 }and anything else. It'll be a very complex lint no matter what. |
This one was pretty tough. I still don't know much about MIR so I likely overcomplicated some of it but I hope it's at least of decent quality :)
I don't know of any FPs (that I haven't fixed already), but there's tons of FNs, most notably
let (x, y) = (1, 1)and the like. It's pretty barebones currently,Closes #4016
changelog: New lint [
local_assigned_single_value]